fix: respect 'always snap to grid' when auto-scale layout from nodes 1.0 to 2.0#9332
Conversation
dosubot
bot
added
the
size:M
📝 WalkthroughWalkthroughA single file enhancement that introduces grid-snapping support to the layout scaling logic by centralizing settings store access, implementing a target-based calculation approach for positions and sizes, and applying conditional snapping across all affected UI elements. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
🎨 Storybook:
|
🎭 Playwright: ⏳ Running... |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.ts (1)
61-65: Consider using a function declaration instead of an arrow function.Per coding guidelines, function declarations are preferred over expressions when possible.
♻️ Suggested refactor
- const applySnap = (pos: [number, number]) => { + function applySnap(pos: [number, number]) { if (alwaysSnapToGrid) { snapPoint(pos, snapSize) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.ts` around lines 61 - 65, Replace the arrow function expression "const applySnap = (pos: [number, number]) => { if (alwaysSnapToGrid) { snapPoint(pos, snapSize) } }" with a function declaration "function applySnap(pos: [number, number]) { if (alwaysSnapToGrid) { snapPoint(pos, snapSize) } }" so it follows the project guideline preferring declarations; keep the same parameter type and body, and ensure any usages of applySnap continue to work (no change to call sites or captured variables like alwaysSnapToGrid and snapSize).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.ts`:
- Around line 61-65: Replace the arrow function expression "const applySnap =
(pos: [number, number]) => { if (alwaysSnapToGrid) { snapPoint(pos, snapSize) }
}" with a function declaration "function applySnap(pos: [number, number]) { if
(alwaysSnapToGrid) { snapPoint(pos, snapSize) } }" so it follows the project
guideline preferring declarations; keep the same parameter type and body, and
ensure any usages of applySnap continue to work (no change to call sites or
captured variables like alwaysSnapToGrid and snapSize).
dosubot
bot
added
size:L
src/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.ts
Outdated
Show resolved
Hide resolved
|
Hi @christian-byrne , thank you for the review! I've implemented the requested changes. |
dosubot
bot
added
size:M
|
I've merged the main branch with #9680 which changed |
christian-byrne
left a comment
christian-byrne
left a comment
There was a problem hiding this comment.
All review feedback addressed. Clean, focused change - snap-to-grid logic is additive with safe defaults, good test coverage for new ceil/floor modes.
christian-byrne
left a comment
christian-byrne
left a comment
There was a problem hiding this comment.
Re-checked after the merge with #9680. The refactored ensureCorrectLayoutScale merged cleanly - snap logic (applySnap helper, unprojectPosSize with graph param, ceil for sizes) is correctly layered on the new simplified structure. Typecheck passes.
2 participants
Summary
Previously when I switch from nodes 1.0 to 2.0, positions and sizes of nodes do not follow 'always snap to grid'. You can guess what a mess it is for people relying on snap to grid to retain sanity. This PR fixes it.
Changes
In
ensureCorrectLayoutScale, we callsnapPointafter the position and the size are updated.We also need to ensure that the snapped size is larger than the minimal size required by the content, so I've added 'ceil' mode to
snapPoint, and the patch is larger than I thought first.I'd happily try out nodes 2.0 once this is addressed :)
┆Issue is synchronized with this Notion page by Unito